[TRA-81] Check isolated market constraints in UpdateSubaccount. #1158
Conversation
WalkthroughThe updates introduce a new market pair Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- protocol/testutil/constants/perpetuals.go (1 hunks)
- protocol/testutil/constants/pricefeed.go (1 hunks)
- protocol/testutil/constants/prices.go (6 hunks)
- protocol/testutil/daemons/pricefeed/exchange_config/market_id.go (1 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (1 hunks)
- protocol/x/subaccounts/types/errors.go (1 hunks)
- protocol/x/subaccounts/types/update.go (2 hunks)
- protocol/x/subaccounts/types/update_test.go (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review due to trivial changes (1)
- protocol/testutil/constants/pricefeed.go
Additional comments: 8
protocol/x/subaccounts/types/errors.go (1)
- 43-46: The addition of
ErrFailedToUpdateIsolatedSubaccountandErrMixedIsolatedCrossUpdatesNotSupportederror variables with codes 600 and 601 respectively, is correctly implemented. These errors are well-defined and follow the existing pattern for error handling within the module.protocol/x/subaccounts/types/update.go (1)
- 60-60: The addition of
ViolatesIsolatedSubaccountConstraintstoupdateResultStringMapand its declaration in theconstblock is correctly implemented. This update result is well-defined and follows the existing pattern for handling update results within the module.protocol/testutil/daemons/pricefeed/exchange_config/market_id.go (1)
- 78-80: The renaming of
MARKET_ISO_USDtoMARKET_ISO2_USDand the addition of a newMARKET_ISO_USDconstant are correctly implemented. These changes ensure that both isolated markets are represented and follow the existing pattern for defining market identifiers.protocol/x/subaccounts/types/update_test.go (1)
- 91-96: The updates to the
TestUpdateResultStringfunction, including the new test case forViolatesIsolatedSubaccountConstraintsand the modification to theUnexpectedErrortest case, are correctly implemented. These changes ensure comprehensive testing of update results and follow the existing pattern for test case handling.protocol/x/subaccounts/keeper/isolated_subaccount.go (1)
- 11-143: The logic introduced for validating updates against isolated market constraints, including the
checkIsolatedSubaccountConstraintsandisValidIsolatedPerpetualUpdatesfunctions, is correctly implemented. These functions are well-defined, follow best practices for error handling and validation, and ensure that updates comply with isolated market constraints in a clear and maintainable manner.protocol/testutil/constants/prices.go (1)
- 289-307: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [30-318]
The addition of the
Iso2UsdPairandIso2UsdExponentconstants, along with updates to test configurations and data to include the new isolated market, are correctly implemented. These changes ensure comprehensive testing of the new isolated market and follow the existing pattern for defining market pairs and test configurations.protocol/testutil/constants/perpetuals.go (1)
- 330-341: The addition of the
Iso2Usd_IsolatedMarketconstant is correctly implemented and aligns with the PR's objectives to support a new isolated market. The parameters chosen for this market, including itsMarketTypeasPERPETUAL_MARKET_TYPE_ISOLATED, are appropriate and consistent with the protocol's design for isolated markets.protocol/x/subaccounts/keeper/subaccount.go (1)
- 295-300: The addition of
checkIsolatedSubaccountConstraintswithinUpdateSubaccountsintroduces a crucial validation step for ensuring updates to subaccounts comply with isolated market constraints. This is a significant enhancement aligning with the PR's objectives to enforce isolated market rules rigorously. However, it's essential to ensure that the implementation ofcheckIsolatedSubaccountConstraintsis robust, efficiently handles edge cases, and is well-documented to facilitate future maintenance and understanding of the logic involved.
Ensure comprehensive testing of this function, especially focusing on edge cases and scenarios where isolated market constraints could be violated.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testutil/constants/pricefeed.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testutil/constants/pricefeed.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/testutil/constants/prices.go (6 hunks)
- protocol/x/prices/keeper/msg_server_update_market_prices_test.go (11 hunks)
- protocol/x/prices/keeper/smoothed_price_test.go (3 hunks)
- protocol/x/prices/keeper/update_price_test.go (3 hunks)
- protocol/x/prices/keeper/validate_market_price_updates_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testutil/constants/prices.go
Additional comments: 13
protocol/x/prices/keeper/smoothed_price_test.go (3)
- 62-62: The addition of
constants.MarketId4to theexpectedResultmap in the test case "Mixed updates and additions - mix of present and missing index prices, smoothed prices" correctly reflects the inclusion of a new market ID for testing. This change ensures that the test coverage is extended to include the newly supported market, aligning with the PR objectives to enhance isolated market constraint handling.- 85-86: The inclusion of
constants.MarketId4in theexpectedErrstring for the test case "Interpolation errors - returns error" is appropriate. It ensures that the error messages are accurately tested for the new market ID, maintaining the integrity of error handling in scenarios where interpolation fails. This change is crucial for validating the robustness of the error handling mechanism for isolated markets.- 100-100: Adding
constants.MarketId4to theexpectedResultmap in the test case "Single interpolation error - returns error, continues updating other markets" is a necessary update to test the behavior of the system when interpolation errors occur for specific markets. It demonstrates the system's ability to continue updating other markets despite encountering errors, which is vital for ensuring system resilience.protocol/x/prices/keeper/update_price_test.go (3)
- 204-204: The addition of
MarketId4withPrice3in theexpectedMsgfor the test case "Multiple market price updates, some from smoothed price and some from index price" is a significant update. It ensures that the new market ID is considered in the validation of market price updates, which is essential for testing the system's ability to handle updates across multiple markets, including isolated ones.- 236-236: Including
MarketId4in theexpectedMsgfor the test case "Mix of valid and invalid index prices" correctly extends the test coverage to include the new market ID. This change is crucial for ensuring that the system accurately processes a mix of valid and invalid market price updates, reflecting the PR's objective to enhance isolated market constraint handling.- 255-255: The addition of
MarketId4in theexpectedMsgfor the test case "Mix of valid, invalid, and invalid historical smoothed prices" is appropriate. It ensures that the system's ability to handle a variety of market price update scenarios, including those involving new market IDs, is thoroughly tested. This change aligns with the PR's goal of improving the protocol's support for isolated markets.protocol/x/prices/keeper/msg_server_update_market_prices_test.go (4)
- 51-51: The inclusion of
constants.MarketId4in theexpectedMarketToPriceInStatemap for the test case "Empty updates" in theTestUpdateMarketPrices_Validfunction is a necessary update. It ensures that the test coverage includes the new market ID, which is crucial for verifying that the system correctly maintains the state of market prices, including those for newly supported markets.- 62-62: Adding
constants.MarketId4with its corresponding price value in theexpectedMarketToPriceInStatemap for the test case "Multiple updates" in theTestUpdateMarketPrices_Validfunction correctly extends the test coverage to include the new market ID. This change is essential for testing the system's ability to handle multiple market price updates accurately, including those involving isolated markets.- 86-86: The inclusion of
constants.MarketId4in theexpectedMarketToPriceInStatemap for various test cases within theTestUpdateMarketPrices_Validfunction, such as "Towards index price = true (current < update < index price)", is appropriate. It ensures comprehensive test coverage, including scenarios where market prices are updated towards the index price.- 236-236: Including
constants.MarketId4in theexpectedMarketToPriceInStatemap for the test case "Index price does not exist, but still updates state" in theTestUpdateMarketPrices_SkipNonDeterministicCheck_Validfunction is a crucial update. It ensures that the test coverage includes scenarios where index prices do not exist for new market IDs, which is vital for testing the system's robustness in handling non-deterministic checks.protocol/x/prices/keeper/validate_market_price_updates_test.go (3)
- 377-377: The addition of
constants.MarketId4to theexpectedMarketIdsslice in the test case "Empty proposed updates, Non-empty local updates" correctly reflects the update to include a new market ID, ensuring the test aligns with the updated market configurations.- 395-395: The inclusion of
constants.MarketId4in themsgUpdateMarketPricesslice for the test case "Non-empty proposed updates, Non-empty local updates, single missing market" is appropriate. It ensures that the test case accurately represents the scenario where all but one market ID is present in the proposed updates, thereby testing the functionality of identifying missing market IDs effectively.- 408-408: Adding
constants.MarketId4to theexpectedMarketIdsslice in the test case "Non-empty proposed updates, Non-empty local updates, multiple missing markets, sorted" is correct. This change ensures that the test case accurately reflects the updated market configurations and tests the functionality of identifying multiple missing market IDs in a sorted manner.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/daemons/liquidation/client/grpc_helper_test.go (1 hunks)
Additional comments: 1
protocol/daemons/liquidation/client/grpc_helper_test.go (1)
- 414-414: The addition of
constants.TestMarketPrices[4]to theMarketPricesslice in theTestGetAllMarketPricesfunction is a positive change, as it likely extends the test coverage to include more market scenarios. However, it's crucial to ensure that this addition aligns with the specific objectives of testing isolated market constraints. Consider verifying the following:
- The relevance of the added market price to isolated market scenarios.
- The comprehensive coverage of isolated market constraints in the test suite.
vincentwschau
changed the title
| @@ -292,6 +292,13 @@ func (k Keeper) UpdateSubaccounts( | |||
| perpIdToFundingIndex[perp.Params.Id] = perp.FundingIndex | |||
| } | |||
|
|
|||
| // Check if the updates satisfy the isolated perpetual constraints. | |||
| success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( | |||
There was a problem hiding this comment.
Should we put this in internalCanUpdateSubaccounts? IIRC, we try to maintain the constraint that CanUpdateSubaccount success guarantees UpdateSubaccounts success on the same input updates. This will no longer hold true if the input updates fails checkIsolatedSubaccountConstraints, which is only checked in UpdateSubaccounts
Either way, should probably put this check before the collateralization check logic in internalCanUpdateSubaccounts to short-circuit those expensive checks
There was a problem hiding this comment.
CanUpdateSubaccounts has a different constraint from UpdateSubaccounts which is why I decided to pull this function into UpdateSubaccounts and not CanUpdateSubaccounts.
CanUpdateSubaccounts does not require unique subaccounts in updates, but UpdateSubaccounts does.
There may not be a use of this specific behavior of CanUpdateSubaccounts, I can check, however I preferred changing UpdateSubaccounts rather than CanUpdateSubaccounts after seeing this difference between the 2 functions.
There was a problem hiding this comment.
CanUpdateSubaccounts is called in AddOrderToOrderbookCollatCheck
Would this mean that an order violating constraints can potentially be go on the orderbook?
There was a problem hiding this comment.
Yes, that's true that it can be added to the orderbook since AddOrderToOrderbookCollatCheck calls CanUpdateSubaccounts. However this is also a path where having a set of updates with non-unique subaccounts are run through CanUpdateSubaccounts but never through UpdateSubaccounts and just adding the above check to CanUpdateSubaccounts would break a working flow.
There was a problem hiding this comment.
Removed the unique subaccount check in checkIsolatedSubaccountConstraints and moved into internalCanUpdateSubaccounts. Specified in comments that the function does not consider constraints across multiple updates.
| "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types" | ||
| ) | ||
|
|
||
| // checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against |
There was a problem hiding this comment.
For my understanding, do we ever expect these constraints to fail during normal operations? If isolated subaccounts is abstracted away from the user, then upstream x/clob logic would be responsible for populating the updates correctly right? So this is more of an internal sanity check?
There was a problem hiding this comment.
Upstream x/clob logic relies on UpdateSubaccounts to determine if the updates to a subaccount for a match are valid. This isn't an internal sanity check, this would be the function / logic that would ensure orders which result in a match that would lead to an invalid subaccount state (based on these new constraints from isolated markets) result in an error.
So during "normal" operations, it is possible for a user with an isolated subaccount to place an order for a different perpetual. Only when a match occurs, would the update fail and the order fail.
There is additional non-scoped work to add logic to x/clob to prevent users from even sending orders that would lead to matches that are invalid based on the isolated market constraints.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/isolated_subaccount.go
| @@ -292,6 +292,13 @@ func (k Keeper) UpdateSubaccounts( | |||
| perpIdToFundingIndex[perp.Params.Id] = perp.FundingIndex | |||
| } | |||
|
|
|||
| // Check if the updates satisfy the isolated perpetual constraints. | |||
| success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( | |||
There was a problem hiding this comment.
CanUpdateSubaccounts is called in AddOrderToOrderbookCollatCheck
Would this mean that an order violating constraints can potentially be go on the orderbook?
|
|
||
| // There cannot be more than a single perpetual update if an update to an isolated perpetual | ||
| // exists in the slice of perpetual updates. | ||
| if hasIsolatedUpdate && len(settledUpdate.PerpetualUpdates) > 1 { |
There was a problem hiding this comment.
Is it possible we have multiple perpetual update entries with the same isolated perpetual?
| StillUndercollateralized: "StillUndercollateralized", | ||
| WithdrawalsAndTransfersBlocked: "WithdrawalsAndTransfersBlocked", | ||
| UpdateCausedError: "UpdateCausedError", | ||
| ViolatesIsolatedSubaccountConstraints: "ViolatesIsolatedSubaccountConstraints", |
There was a problem hiding this comment.
Is this how the error gets propagated to the client? Do we think we need to provide more info or would this be sufficient?
There was a problem hiding this comment.
There's several more layers of indirection before this error is sent to the client. I'm planning on looking at the error propagation once integration tests / more E2E testing is added.
Agree on adding more integration / E2E testing. Will add a TODO for this, however I don't want to include and increase scope of this PR. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/subaccounts/keeper/isolated_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/keeper/subaccount_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/subaccounts/keeper/subaccount.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/subaccount.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/app/e2e/ante_test.go (1 hunks)
Check Runs (5)
install-runsim completed (5)
validate-yaml completed (1)
build completed (1)
exchange-tests completed (1)
labeler completed (1)
Additional comments: 1
protocol/app/e2e/ante_test.go (1)
- 140-140: The parameter value in the test function
TestParallelAnteHandler_ClobAndOtherhas been changed from100_000to110_000. Could you please clarify the rationale behind this change? It's important to understand how this adjustment affects the test's sensitivity to transaction processing limits or fees and whether it aligns with the objectives of enhancing subaccount management by validating updates against isolated market constraints.
| @@ -511,6 +517,7 @@ func checkPositionUpdatable( | |||
|
|
|||
| // internalCanUpdateSubaccounts will validate all `updates` to the relevant subaccounts. | |||
| // The `updates` do not have to contain `Subaccounts` with unique `SubaccountIds`. | |||
| // The `updates` do not have to contain `Subaccounts` with unique `SubaccountIds`. | |||
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.go
Changelist
Adds logic to check whether updates passed into
UpdateSubaccountare valid under the isolated markets constraints.Test Plan
Unit tests.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.